Fix Cursor::{next,previous}_logical_word and skip over whitespace - #215
Conversation
| let [left, right] = self.logical_clusters(layout); | ||
| if let Some(cluster) = left.or(right) { | ||
| let [upstream, downstream] = self.logical_clusters(layout); | ||
| if let Some(cluster) = downstream.or(upstream) { |
There was a problem hiding this comment.
This is interesting as we're checking the same clusters (in logical order) as for the method in the other direction. I believe the asymmetry comes from the word boundary clusters being the logically first cluster of a word and (e.g.) the whitespace logically following the word, e.g., in "foo bar" the boundary clusters of the first word are "f" and " ".
| return Self::from_byte_index(layout, usize::MAX, Affinity::Downstream); | ||
| } | ||
| return Self::from_cluster(layout, cluster, true); | ||
| let moving_right = !cluster.is_rtl(); |
There was a problem hiding this comment.
moving_right in from_cluster calculates the Affinity from the cluster's rtl; to calculate it from the rtl originally is... interesting.
There was a problem hiding this comment.
Yeah... I've simplified this a bit by using the Self::from_byte_index directly with an explicit Affinity, rather than going through the apply-then-unapply directionality dance to get at an affinity. Both next_logical_word and previous_logical_word now affine downstream, which I think is correct, and I've added comments explaining the behavior (and at least, manually wiring these methods up in the editor to visually inspect their behavior without mutating text seems to behave as I'd expect (and on main only backdelete calls prev_logical_word, so it's also not clear whether the affinity really matters at all here)).
| let focus = if let Some(end) = cluster.next_logical_word() { | ||
| Cursor::from_cluster(layout, end.clone(), !cluster.is_rtl()) | ||
| } else { | ||
| Cursor::from_byte_index(layout, usize::MAX, Affinity::Downstream) | ||
| }; |
There was a problem hiding this comment.
With the new whitespace skipping, double-click-to-select can't use Cursor::next_logical_word anymore. Note the anchor already used Cluster::previous_logical_word, as opposed to focus that was using Cursor::next_logical_word (note: Cluster vs. Cursor as the type!).
Cluster uses the Unicode word boundaries from analysis, whereas Cursor encodes "editor behavior." When selecting a word in an editor by double clicking, you do want to stop at whitespace. Hence, this should use Cluster.
tomcur
changed the title
Cursor::previous_logical_wordCursor::{next_,previous_}_logical_word and skip over whitespace
tomcur
changed the title
Cursor::{next_,previous_}_logical_word and skip over whitespaceCursor::{next,previous}_logical_word and skip over whitespace
|
This PR has changed a bit so probably needs re-review. The fix I originally proposed regressed whitespace handling. This now also includes a fix for #604, as I believe that's the only clean way to get this to behave as expected for both cases, e.g., |
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
To clarify my understanding, these methods now jump to the "logical start" of the next or previous logical word, right?
That seems as reasonable as anything, but we probably should document it?
Currently, in `foo b|ar` where `|` indicates the cursor position,
`Cursor::previous_logical_word` will return `foo| bar`. This doesn't
match the behavior of `Cursor::{next_logical_word,
previous_visual_word}` which place the cursor at the boundary of the
current word. This happens because `Cluster::previous_logical_word` is
called from the upstream cluster (the "b") and the cursor then lands
between the "o" and the space.
This also renames "left", "right" -> "upstream", "downstream" (naming
copied from `Cursor::logical_clusters`) to make it clear we're operating
in logical order and not visual, and affines the cursor towards the word
whose boundary it lands on.
(Note the behavior between `{previous, next}_logical_word` and
{previous, next}_visual_word` aren't quite the same yet: the `visual`
methods jump over whitespace, the `logical` ones don't.)
2 participants
Currently, in
foo b|arwhere|indicates the cursor position,Cursor::previous_logical_wordwill returnfoo| bar. This doesn't match the behavior ofCursor::{next_logical_word, previous_visual_word}which place the cursor at the boundary of the current word. This happens becauseCluster::previous_logical_wordis called from the upstream cluster (the "b") and the cursor then lands between the "o" and the space.This also renames "left", "right" -> "upstream", "downstream" (naming copied from
Cursor::logical_clusters) to make it clear we're operating in logical order and not visual, and attaches the cursor to the word whose boundary it lands on.The upstream/downstream fix on its own with no other changes, would regress how whitespace is handled, e.g., in
foo |bar, whereprevious_logical_wordwould then end up atfoo| barinstead of|foo bar. The cleanest way to handle this, I think, is to fold in a fix for #604 at the same time, skipping over whitespace. That's what this PR now proposes. This aligns these logical jumps to the visual{previous, next}_visual_word.